Repository navigation
refactor(playground): remove session token - #3965
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe playground no longer uses per-session URL tokens or token-based API authorization. Rust startup and server paths, frontend URL construction, token exports, and WebSocket connection-status handling were simplified accordingly. ChangesPlayground access and client integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
typescript2/app-vscode-webview/src/App.tsx (1)
109-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWS fallback hardcodes
ws://; derive scheme fromwindow.location.protocol.
remoteUrlsFromLocationinremote-backend.tspickswssforhttps:origins, but this fallback always usesws://. If the standalone/iframe page is ever served over HTTPS, the browser will block the insecurews://connection as mixed content. Align the two paths.♻️ Proposed fix to match protocol-aware scheme selection
- const wsUrl = - window.__PLAYGROUND_WS_URL ?? `ws://${window.location.host}/api/ws`; + const scheme = window.location.protocol === 'https:' ? 'wss' : 'ws'; + const wsUrl = + window.__PLAYGROUND_WS_URL ?? `${scheme}://${window.location.host}/api/ws`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/app-vscode-webview/src/App.tsx` around lines 109 - 110, Update the `wsUrl` fallback in `App` to derive the WebSocket scheme from `window.location.protocol`, using `wss` for HTTPS and `ws` otherwise, while preserving the existing host and `/api/ws` path. Keep the explicit `window.__PLAYGROUND_WS_URL` override unchanged and align the logic with `remoteUrlsFromLocation`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@typescript2/app-vscode-webview/src/App.tsx`:
- Around line 109-110: Update the `wsUrl` fallback in `App` to derive the
WebSocket scheme from `window.location.protocol`, using `wss` for HTTPS and `ws`
otherwise, while preserving the existing host and `/api/ws` path. Keep the
explicit `window.__PLAYGROUND_WS_URL` override unchanged and align the logic
with `remoteUrlsFromLocation`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 819738a0-286e-40ca-bcb3-d13625b3129e
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_keyword_playground.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
baml_language/crates/baml_builtins2/keyword_docs/baml_keywords.yamlbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/lib.rsbaml_language/crates/baml_lsp_server/src/playground_sender.rsbaml_language/crates/baml_lsp_server/src/playground_server.rstypescript2/app-vscode-webview/src/App.tsxtypescript2/app-vscode-webview/src/RemoteEditorView.tsxtypescript2/app-vscode-webview/vite.config.tstypescript2/pkg-editor/package.jsontypescript2/pkg-editor/src/index.tstypescript2/pkg-editor/src/playground-token.tstypescript2/pkg-editor/src/remote-backend.tstypescript2/pkg-playground/src/index.tstypescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
💤 Files with no reviewable changes (8)
- typescript2/pkg-editor/src/playground-token.ts
- baml_language/crates/baml_lsp_server/Cargo.toml
- typescript2/pkg-playground/src/index.ts
- typescript2/pkg-editor/src/index.ts
- typescript2/app-vscode-webview/vite.config.ts
- baml_language/crates/baml_builtins2/keyword_docs/baml_keywords.yaml
- typescript2/pkg-editor/package.json
- typescript2/pkg-playground/src/ports/WebSocketRuntimePort.ts
# Conflicts: # baml_language/crates/baml_lsp_server/src/lib.rs # baml_language/crates/baml_lsp_server/src/playground_sender.rs # baml_language/crates/baml_lsp_server/src/playground_server.rs
…ment + Phase 1 bounded ingress (BoundaryML#4000) Combines BoundaryML#3969 (Phase 0) and BoundaryML#3976 (Phase 1) into one PR against canary, with latest canary merged in (including the BoundaryML#3965 session-token removal and BoundaryML#3973 test-metadata work, both semantically reconciled). Supersedes both stacked PRs. Implements `docs/design/lsp-latency-and-mutex-fix.md` (committed in this PR) through Phase 1; Phase 2 (salsa snapshot reads) remains behind the doc's spike gate. ## Root cause The 0.14.0 debounced engine rebuild holds the project DB mutex across compiles while `try_lock_db` maps any contention to `-32001`; stale build candidates could install over newer edits, busy diagnostic reads looked empty, and playground/browser routing could leak stale session output. ## Phase 0 — correctness containment (from BoundaryML#3969) - Monotonic `SourceRevision`; owned compilation/diagnostic/engine candidates; **revision-conditional engine commit** through the same gate as source mutations — stale installs impossible by construction (rendezvous-tested at the exact commit boundary). - `DiagnosticsFence` judging staleness against the **authoritative revision**: a superseded rebuild's late diagnostics or a candidate racing `mark_dirty` can never regress newer markers. - Bounded request-lane waits (1s) mapping to typed errors — `ContentModified`/`RequestFailed`; **`-32001` is never emitted**. Busy is never conflated with empty; poison is terminal. - Salsa-memoized `file_annotations`/`semantic_tokens`; 150/300 ms debounced tails; single-flight rebuilds; UTF-8/UTF-16 `PositionCodec` (C1). ## Phase 1 — bounded ingress, cancellation, client gating (from BoundaryML#3976) - **IngressScheduler + LspRuntime** in `baml_lsp_server`: stdio and browser LSP sessions through one bounded scheduler (per-class byte/item budgets, lifecycle barriers, response ownership); `$/cancelRequest` handled on the transport thread — queued requests answer **`-32800`**, running cancellable requests observe a dispatch-scoped token (entry + post-source-guard safe points; never salsa unwinding); browser takeover tombstones the old session's sink and replays overlays. - Hardened framing/output: 16 KiB header cap; oversized messages/responses degrade **per-message** (`-32803`) instead of killing the session/process; serialize-once shared outbound frames with accurate budget accounting; connection-scoped sessions (per-connection encoding negotiation). - **Playground run-gating (D6)**: during rebuild windows (`is_bex_current == false` or a `projectNotReady` rejection) Run/test/preview disable with "Preparing current build…" and re-enable on the next current update — no protocol version bump. Closes the fail-closed D7 UX window Phase 0 introduces. - **Reconnect correctness**: fail-closed hello; bounded first-connect queue; session-scoped commands never replay into a new session (synthesized rejections); the desired-runtime lease survives reconnects. - **Track A**: canonical multi-root ownership (`projectRoots.ts`) + routing; closing the last `.baml` document no longer stops the owner server; aggregate status preserves startup failures. ## Measured (same harness/machine, 400 generated functions) | Metric | 0.14.1 (regressed) | this PR | |---|---:|---:| | Typing inlayHint p50 / p95 | 313.6 / 379.4 ms | **~2 / ~52 ms** | | Post-pause `-32001` failures | 46–57 per feature/window | **0** | | Unanswered requests | 144–160 | **0** | | Rebuild-window inlay/hover/completion | mass failures | 218/218 each | | Explicit `$/cancelRequest` | rejected/ignored | **`-32800`** | | Refresh storms per 10-edit burst | — | 0 | ## Deliberately not included - **Phase 2 salsa snapshots** (removes the residual post-pause hitch on very large projects): gated by the design doc's own spike-and-signoff decision — `panic=abort` release profile vs salsa's unwinding cancellation must be resolved first. - **D5 demand-gating server side**: client seam ships dormant (typed + tested, nothing sends it). - The alternate implementation's five-lock core rewrite (redundant with Phase 0's invariants), playground protocol-v3 lockstep (compat hazard; equivalent wire-compatible fixes included), and stdlib `testing.*` API additions (unauthorized surface). ## Validation - `bex_project` 45/45 (+1 pre-existing ignored) incl. commit-boundary rendezvous, fence, bounded-wait classification; `baml_lsp_server` 64/64 (22 scheduler, 9 runtime, framing/budget); workspace `cargo check --lib` clean; clippy `-D warnings` clean; wasm check clean. - TS: playground 125/125, extension 13/13, webview 19/19 (incl. gating), all typechecks. - Latency harness re-run on this exact merged head (numbers above). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01VEMxti5WQ8bCgJUuX9AQvo <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved LSP responsiveness and stability with bounded messaging, cancellation handling, session recovery, and safer browser takeover behavior. * Added negotiated UTF-8/UTF-16 position handling for more accurate diagnostics, ranges, formatting, and semantic highlighting. * Added project-aware VS Code routing for nested projects, symlinks, and changing project markers. * Added runtime leasing support for playground connections. * **Bug Fixes** * Prevented runs and tests from starting while a project build is stale or unavailable, with clearer preparation feedback. * Improved cancellation, error reporting, diagnostics updates, and run generation consistency. * **Documentation** * Expanded contributor and Rust workspace setup, testing, formatting, and debugging guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5 <[email protected]>
Summary
?token=propagation.Why
#3949 introduced the session token alongside the SSH playground changes. We have decided not to use token-based authorization for the local playground, so this removes the feature and all supporting structures rather than leaving compatibility plumbing or dead code behind.
Impact
baml playgroundnow prints and opens a bare local URL. Playground API and WebSocket requests no longer require a query parameter. The origin guard remains in place: remapped loopback origins used by SSH tunnels are accepted, while hostile remote origins are rejected.Validation
cargo fmt --all -- --checkcargo test -p baml_lsp_server --lib— 24 passedcargo test -p baml_cli describe_command_tests— 89 passedpnpm --filter @b/pkg-playground typecheckpnpm --filter @b/pkg-proto generate && pnpm --filter app-vscode-webview buildSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation